Skip to content

feat: Suport Node.js crypto KeyObjects #200

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Aug 31, 2019

Conversation

seebees
Copy link
Contributor

@seebees seebees commented Aug 28, 2019

resolves #74

KeyObjects were introduced in Node.js v11.
They wrap the Buffer in an object that has
even better security properties than an isolated buffer.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Check any applicable:

  • Were any files moved? Moving files changes their URL, which breaks all hyperlinks to the files.

resolves aws#74

KeyObjects were introduced in Node.js v11.
They wrap the Buffer in an object that has
even better security properties than an isolated buffer.
@seebees seebees requested a review from a team August 28, 2019 04:30
@@ -212,10 +216,17 @@ export function nodeKdf (material: NodeEncryptionMaterial|NodeDecryptionMaterial
info instanceof Uint8Array,
'Invalid HKDF values.'
)
/* The unwrap is done once we *know* that a KDF in required.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/in/is/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@@ -65,3 +65,27 @@ export type DecryptionMaterial<Suite> =
Suite extends NodeAlgorithmSuite ? NodeDecryptionMaterial :
Suite extends WebCryptoAlgorithmSuite ? WebCryptoDecryptionMaterial :
never

export type AwsEsdkKeyObjectType = 'secret' | 'public' | 'private'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of these exports?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add comments. Sorry. This is because there are different versions of Node.js. If I use/export the v11/v12 version then Typescript will complain if consumers of this package use any other version of Node.js (and we must support older versions.)

@@ -163,7 +180,14 @@ describe('decorateCryptographicMaterial', () => {
material.setUnencryptedDataKey(dataKey, { keyNamespace: 'k', keyName: 'k', flags: KeyringTraceFlag.WRAPPING_KEY_GENERATED_DATA_KEY })
const test = material.getUnencryptedDataKey()
test[0] = 12
expect(() => material.getUnencryptedDataKey()).to.throw()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand the code below, we are testing that one way to try to modify a KeyObject udk doesn't work. The purpose of this test should be to test that if it does get modified, then we fail. If it should be impossible to modify the KeyObject, then perhaps the below is fine for the KeyObject case, however I still think we should check that is the udk is not a KeyObject then it still correctly fails if modified.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the non-KeyObject case getUnencryptedDataKey will throw before the dataKey is even returned. In this case we are dealing with Uint8Array (raw memory). The cryptographic material keeps an independent copy of the dataKey and compares it to the dataKey that it is going to return.

This is because all returned instances are references to the same memory so anyone could have manipulated the memory.

@seebees seebees merged commit 77ad031 into aws:master Aug 31, 2019
@seebees seebees deleted the support-keyobjects branch August 31, 2019 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support KeyObjects in Node.js
2 participants